Add Tacotron2 loss function #1625

yangarbiter · 2021-07-10T00:24:04Z

The main Tacotron2 model is in #1621. Since that PR is already too large, I'll put the Tacotron2 loss and it's tests in the example directory temporary. We should discuss more on where to put this loss function (likely the same file as Tacotron2's model).

carolineechen · 2021-07-13T15:50:26Z

examples/pipeline_tacotron2/loss_function.py

+
+class Tacotron2Loss(nn.Module):
+    """Tacotron2 loss function adapted from:
+    https://github.com/NVIDIA/DeepLearningExamples/blob/master/PyTorch/SpeechSynthesis/Tacotron2/tacotron2/loss_function.py


can you also add a reference to the original paper?

Thanks for pointing it out. I've addressed it here.

carolineechen · 2021-07-13T15:53:00Z

examples/pipeline_tacotron2/loss_function.py

@@ -0,0 +1,73 @@
+# *****************************************************************************


I think it makes sense to put this with the Tacotron2 model files for now, and same goes for the unit test files.

According to the internal document, it appears that we should implement it in examples folder first. We can then discuss whether we want to merge it into the core later on.
Please let me know how you think about it. :)
Thanks.

carolineechen · 2021-07-13T15:55:42Z

examples/pipeline_tacotron2/test_tacotron2_loss.py

+class Tacotron2LossTest(unittest.TestCase, TempDirMixin):
+
+    dtype = torch.float64
+    device = "cpu"


Is there a reason why only cpu is being tested?

I've add GPU tests here and here.
Thanks for pointing it out!

vincentqb

please move all the files related to the loss (e.g. loss_function.py and tests) in a subfolder examples/pipeline_tacotron2/loss/

vincentqb · 2021-07-13T18:34:58Z

examples/pipeline_tacotron2/loss_function.py

+                (2) predicted mel spectrogram after the postnet (mel_specgram_postnet)
+                    with shape (n_batch, n_mel, n_time), and
+                (3) the stop token prediction (gate_out) with shape (n_batch).
+            targets (tuple of two Tensors): The ground truth mel spectrogram (n_batch, n_mel, n_time) and


nit: we write `(batch, mel, time), see here

Thanks.
Addressed here

yangarbiter · 2021-07-13T21:04:30Z

Following @vincentqb's suggestion, I've also run the code through black.

vincentqb · 2021-07-14T17:58:11Z

examples/pipeline_tacotron2/loss/test_tacotron2_loss.py

+    )
+
+
+class Tacotron2LossTest(unittest.TestCase, TempDirMixin):


please follow the convention we have for testing cpu/gpu, and torchscript/autograd.

one base class for torchscript + two subclasses for cpu/gpu

one base class for autograd + two subclasses for cpu/gpu

please also add a test that validates the shape of the output.

Thanks for the suggestion.
The shape test is here.
And the base class and subclasses is also addressed in the same commit.

vincentqb · 2021-07-14T18:04:41Z

examples/pipeline_tacotron2/loss/test_tacotron2_loss.py

+if __name__ == "__main__":
+    unittest.main()


please remove this as we do not have this in test files anymore

Thanks for the suggestion, it is removed.

vincentqb · 2021-07-14T18:14:32Z

examples/pipeline_tacotron2/loss/test_tacotron2_loss.py

+
+
+def _get_inputs(dtype, device):
+    n_mel, n_batch, max_mel_specgram_length = 3, 2, 4


these seem very small and are not representative of a typical tensor shape. please make n_mel and _max_mel_specgram_length larger, following examples from the other tests.

vincentqb

LGTM. @carolineechen @mthrok -- do you have any other feedback?

mthrok

How would you like to treat the test? We can run them as a a part of CI.
Also can you document how to run the test?

yangarbiter · 2021-07-22T21:41:58Z

How would you like to treat the test? We can run them as a a part of CI.

Yes, I think it is a good idea to run them as part of CI. Could you direct me on how to do that properly?
Thanks.

Should I edit the code here by adding

cd ../examples/pipeline_tacotron2/
pytest pytest "${args[@]}" .
cd ../../test

I use the whole directory (pipeline_tacotron2) because there are going to be tests for text utilities too.

Also can you document how to run the test?

Sure, I've added a short description at examples/pipeline_tacotron2/README.md

mthrok · 2021-07-26T17:06:00Z

How would you like to treat the test? We can run them as a a part of CI.

Yes, I think it is a good idea to run them as part of CI. Could you direct me on how to do that properly?
Thanks.

Should I edit the code here by adding
cd ../examples/pipeline_tacotron2/
pytest pytest "${args[@]}" .
cd ../../test
I use the whole directory (pipeline_tacotron2) because there are going to be tests for text utilities too.

Also can you document how to run the test?

Sure, I've added a short description at examples/pipeline_tacotron2/README.md

Ideally, we want to run the test in a separate job, but for a starter, this is good. You do not need "${args[@]}" as that is mostly intended for unit test. I would simply add -v. i.e. pytest -v .

yangarbiter · 2021-07-26T18:01:46Z

How would you like to treat the test? We can run them as a a part of CI.

Yes, I think it is a good idea to run them as part of CI. Could you direct me on how to do that properly?
Thanks.
Should I edit the code here by adding
cd ../examples/pipeline_tacotron2/
pytest pytest "${args[@]}" .
cd ../../test
I use the whole directory (pipeline_tacotron2) because there are going to be tests for text utilities too.

Also can you document how to run the test?

Sure, I've added a short description at examples/pipeline_tacotron2/README.md
Ideally, we want to run the test in a separate job, but for a starter, this is good. You do not need "${args[@]}" as that is mostly intended for unit test. I would simply add -v. i.e. pytest -v .

Thanks for the comment.
The tests are add in .circleci/unittest/linux/scripts/run_test.sh and .circleci/unittest/windows/scripts/run_test.sh.

mthrok · 2021-07-26T18:10:17Z

examples/pipeline_tacotron2/loss/tacotron2_loss_gpu_test.py

+)
+
+
+def skipIfNoCuda(test_item):


~~Can you add #TODO: Find a way to de-duplicate this utility and reuse the original one from "test/torchaudio_unittest"~~

~~Looks like multiple utilities are duplicated, and this soon becomes unmanageable. Can you modify PYTHONPATH to include the <repo_root>/test/ directory so that you can import?~~

Actually, I remembered that example directories are exposed in unit tests.

audio/test/torchaudio_unittest/example/__init__.py

Lines 1 to 8 in c49db73

import os

import sys

sys.path.append(

os.path.join(

os.path.dirname(__file__),

'..', '..', '..', 'examples'))

Can you move the test implementations into test/torchaudio_unittest/example directory?

Good idea! I've moved the test implementations to test/torchaudio_unittest/example/tacotron2.

examples/pipeline_tacotron2/loss/__init__.py

.circleci/unittest/linux/scripts/run_test.sh

facebook-github-bot added the CLA Signed label Jul 10, 2021

yangarbiter marked this pull request as draft July 10, 2021 00:24

yangarbiter force-pushed the tacotron2_loss branch 2 times, most recently from f65959d to b72489c Compare July 12, 2021 16:39

Add Tacotron2 loss function

b217afd

yangarbiter force-pushed the tacotron2_loss branch from b72489c to b217afd Compare July 12, 2021 18:27

yangarbiter requested review from vincentqb, mthrok and carolineechen July 12, 2021 21:12

carolineechen reviewed Jul 13, 2021

View reviewed changes

vincentqb reviewed Jul 13, 2021

View reviewed changes

yangarbiter force-pushed the tacotron2_loss branch from 42b866c to f9b3e64 Compare July 13, 2021 21:03

yangarbiter force-pushed the tacotron2_loss branch from f9b3e64 to cbbef8e Compare July 13, 2021 21:07

Refactor the code

3365afa

yangarbiter force-pushed the tacotron2_loss branch from cbbef8e to 3365afa Compare July 13, 2021 21:15

vincentqb reviewed Jul 14, 2021

View reviewed changes

yangarbiter force-pushed the tacotron2_loss branch 2 times, most recently from 5b9942b to 1e16e29 Compare July 14, 2021 20:00

refactor tacotron2 loss tests

1996fec

yangarbiter force-pushed the tacotron2_loss branch from 1e16e29 to 1996fec Compare July 14, 2021 20:03

vincentqb approved these changes Jul 14, 2021

View reviewed changes

yangarbiter marked this pull request as ready for review July 15, 2021 16:58

mthrok reviewed Jul 22, 2021

View reviewed changes

Add instructions for running tests for tacotron2 loss

7c522f0

mthrok reviewed Jul 26, 2021

View reviewed changes

yangarbiter force-pushed the tacotron2_loss branch 4 times, most recently from 62e4727 to bd6684a Compare July 26, 2021 19:58

Add the test for tacotron2 loss to CI

b6288bc

yangarbiter force-pushed the tacotron2_loss branch from bd6684a to b6288bc Compare July 26, 2021 21:06

mthrok approved these changes Jul 26, 2021

View reviewed changes

yangarbiter merged commit 1b52e72 into pytorch:master Jul 26, 2021

yangarbiter mentioned this pull request Jul 26, 2021

Refactor text preprocessing tests in Tacotron2 example #1641

Merged

nateanl pushed a commit to nateanl/audio that referenced this pull request Jul 28, 2021

Add Tacotron2 loss function (pytorch#1625)

12f13fe

		@@ -0,0 +1,73 @@
		# *****************************************************************************



		def _get_inputs(dtype, device):
		n_mel, n_batch, max_mel_specgram_length = 3, 2, 4

	import os
	import sys


	sys.path.append(
	os.path.join(
	os.path.dirname(__file__),
	'..', '..', '..', 'examples'))

Add Tacotron2 loss function #1625

Add Tacotron2 loss function #1625

Uh oh!

Conversation

yangarbiter commented Jul 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangarbiter Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentqb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangarbiter commented Jul 13, 2021

Uh oh!

vincentqb Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentqb left a comment

Choose a reason for hiding this comment

Uh oh!

mthrok left a comment

Choose a reason for hiding this comment

Uh oh!

yangarbiter commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mthrok commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yangarbiter commented Jul 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yangarbiter commented Jul 10, 2021 •

edited

Loading

yangarbiter Jul 13, 2021 •

edited

Loading

vincentqb Jul 14, 2021 •

edited

Loading

yangarbiter commented Jul 22, 2021 •

edited

Loading

mthrok commented Jul 26, 2021 •

edited

Loading